feat(Codecov): add storage solution #90693
Conversation
github-actions
bot
added
the
Scope: Frontend
adrianviquez
changed the title
❌ 62 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| import {useNavigate} from 'sentry/utils/useNavigate'; | ||
| import useOrganization from 'sentry/utils/useOrganization'; | ||
|
|
||
| // Types |
There was a problem hiding this comment.
Types should explain why, not what :)
There was a problem hiding this comment.
Will adjust all comments
There was a problem hiding this comment.
This shouldn't have been checked in
| export type CodecovContextSetterTypes = { | ||
| setContextState: React.Dispatch<React.SetStateAction<CodecovContextTypes>>; | ||
| updateSelectorData: (value: Partial<CodecovContextTypes>) => void; | ||
| }; |
There was a problem hiding this comment.
| export type CodecovContextSetterTypes = { | |
| setContextState: React.Dispatch<React.SetStateAction<CodecovContextTypes>>; | |
| updateSelectorData: (value: Partial<CodecovContextTypes>) => void; | |
| }; | |
| export type CodecovContextSetterTypes = { | |
| setContextState: React.Dispatch<React.SetStateAction<CodecovContextTypes>>; | |
| updateSelectorData: (value: Partial<CodecovContextTypes>) => void; | |
| }; |
Why do you need both here? If someone calls setContextState, it means it wont end up updating query params, potentially detaching state.
There was a problem hiding this comment.
Yeah true, I don't need to expose the setContextState fn, that should be private to the context itself. Left it there from the original implementation but no need to have it
| import useOrganization from 'sentry/utils/useOrganization'; | ||
|
|
||
| // Types | ||
| export type CodecovContextTypes = { |
There was a problem hiding this comment.
| export type CodecovContextTypes = { | |
| export type CodecovContext = { |
There is no need to annotate this any further
| integratedOrg: string | null; | ||
| repository: string | null; | ||
| }; | ||
| export type CodecovContextSetterTypes = { |
There was a problem hiding this comment.
Why do you export these separately?
| import type { | ||
| CodecovContextSetterTypes, | ||
| CodecovContextTypes, | ||
| } from 'sentry/components/codecov/container/container'; |
There was a problem hiding this comment.
This should only export a single type.
Also, codecov/container/container isn't good file structure (same with context/context).
It should be something like CodecovParamsProvider.tsx or similar to better explain what it does. The same goes for the provider, it may be obvious to you that CodecovProvider provides params, but to someone reading it for the first time, they'll have no idea what a provider like that does.
There was a problem hiding this comment.
I renamed these to codecovParamsProvider.tsx and codecovContext.tsx. I normally let the folder help me with understanding it, so like codecov/components/context helps me understand it's a codecovContext, but it is true it can be clearer to just name the file codecovSomething
|
|
||
| export function useCodecovContext() { | ||
| const context = useContext(CodecovContext); | ||
| if (!context) throw new Error('useCodecovContext must be used within CodecovProvider'); |
There was a problem hiding this comment.
| if (!context) throw new Error('useCodecovContext must be used within CodecovProvider'); | |
| if (context === undefined) throw new Error('useCodecovContext was called outside of CodecovProvider'); |
This is a nit, but I prefer to target the exact type here.
Wrt err messages, it's always better to explain why the error was thrown, and be precise about what caused it (your message has a more info tone to it)
| const updateSelectorData: CodecovContextSetterTypes['updateSelectorData'] = data => { | ||
| return setContextState(prev => { | ||
| const newState = {...prev, ...data}; | ||
| setSelectorData(newState); | ||
|
|
||
| const currentParams = new URLSearchParams(location.search); | ||
| for (const [key, value] of Object.entries(newState)) { | ||
| if (value !== null) { | ||
| currentParams.set(key, value); | ||
| } | ||
| } | ||
| navigate(`${location.pathname}?${currentParams.toString()}`, {replace: true}); | ||
|
|
||
| return newState; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
You should useCallback here as this is stable. If something updates query params that you don't care about, the rerender will create a new function here and propagate updates to the tree.
|
|
||
| return ( | ||
| <CodecovContext.Provider | ||
| value={{...contextState, setContextState, updateSelectorData}} |
There was a problem hiding this comment.
This is just as info, but a common practice in react to avoid this and separate the data into a getter and a setter is to separate it to a separate context.
| const [contextState, setContextState] = useState(() => { | ||
| // Get query params | ||
| const resultsFromQuery = CODECOV_URL_PARAM.reduce<Record<string, string | null>>( | ||
| (acc, value: string) => { | ||
| const queryParam = queryParams[value]; | ||
| acc[value] = | ||
| typeof queryParam === 'string' ? decodeURIComponent(queryParam).trim() : null; | ||
| return acc; | ||
| }, | ||
| {} | ||
| ); | ||
| const hasAnyNonNull = Object.values(resultsFromQuery).some(value => value !== null); | ||
| if (hasAnyNonNull) { | ||
| return {...DEFAULT_CODECOV_CONTEXT_VALUES, ...resultsFromQuery}; | ||
| } | ||
|
|
||
| // Get data from local storage | ||
| if (selectorData) { | ||
| return {...DEFAULT_CODECOV_CONTEXT_VALUES, ...selectorData}; | ||
| } | ||
|
|
||
| return DEFAULT_CODECOV_CONTEXT_VALUES; | ||
| }); |
There was a problem hiding this comment.
This approach here is brittle, as it means that if someone updates a query param of codecov directly without updateSelectorData, the update wont propagate to this provider and you'll end up with stale values. Imo, this is not what you want, and you should instead encourage people to write directly to query params and treat that as the source of truth (as opposed to state). Since URL is state, why do you need the separate react state for it?
|
@adrianviquez I would suggest to simplify this to not use React state and be resilient to query param updates outside of the provider. function CodecovQueryParamsProvider(props){
const location = useLocation();
const localStorageState = useLocalStorage();
const params = {
key: location.query.key || localStorageState.key || default value
...
}
return <Provider value={params}>{props.children}</Provider>
}The only part left then is to decide how to sync query to localStorage, which you could probably just do from inside an effect inside this provider |
|
Ty for the feedback @JonasBa. I refactored the component to encourage using url as the state. I believe I addressed all of your suggestions, apologies if I missed one. I created a Please let me know what you think about these changes, thanks for all the care and feedback 🙇 |
| ); | ||
| const navigate = useNavigate(); | ||
|
|
||
| const updateSelectorData: CodecovContextData['updateSelectorData'] = useCallback( |
There was a problem hiding this comment.
Thoughts on this?
My intent is having one piece of logic that children using this provider can point to, so I created this fn. That being said, it currently only adds the 1st key/value of data (which is a single Record<string, string> though). So if I wanted to make it more robust, I could loop through the keys of data and add any value.
This PR adds a context to the Codecov product to hold state and functionality to save/get data to/from the url and local storage. This context is used in the CodecovProvider that will be the main wrapper for all things Codecov. This is the preferred approach over this pr.
Notes
CodecovProvider: this is the main code that will act as a wrapper for everything in the Codecov product. It instantiates and initializes the CodecovContext component, as well as sets functions to manipulate the context.CodecovContext: the context itself.Thanks!